Code refactoring to reduce memory footprint#12385
Code refactoring to reduce memory footprint#12385vkuznet wants to merge 4 commits intodmwm:masterfrom
Conversation
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
|
Jenkins results:
|
amaltaro
left a comment
There was a problem hiding this comment.
Valentin, the code itself looks okay to me. However, the improvements to memory footprint will be negligible. In the current state of the system, it improves memory consumption by 0.01% only, as it still loads all requests matching a given request status from:
reqStatus = ['announced', 'aborted-completed', 'rejected']
The unit test probably needs updating, hence my request for changes here.
| def testRunning(self): | ||
| result = self.msRuleCleaner._execute(self.reqRecords) | ||
| self.assertEqual(result, (3, 2, 0, 0)) | ||
| def testMemoryLeak(self): |
There was a problem hiding this comment.
You probably didn't want to persist this memory leak investigation as a unit test (?)
Alan, how to you come up to your conclusion and in particular the number you quote? My understanding that memory now is reused due to this loop: Yes, we loop over all request statuses, but within the loop we re-use I would appreciate if you provide your view on where memory is spent that it would be easier to understand your request. |
|
@vkuznet the previous The only gain is that now we load requests for a single status, instead of loading requests for all eligible statuses (3 in total). In order to really solve the memory footprint, we would have to control how many requests we can fetch from ReqMgr2/CouchDB, as implemented in There are a few ways that we could handle this, but none of them look simple and/or robust to me. For instance: All of them have tradeoffs though. Right now, I suspect b) would be a good commitment. |
|
Alan, thank you for providing more details. The current code used two nested for loops, in one it captured all requests in a dict ( That said, from your details you claim that majority of memory allocation comes from I'll have a look into options you propose, but please clarify further how to discover the workflow names as you propose. |
|
@vkuznet I only did a ratio of max(workflows in (a,b,c)) / sum(workflows in (a,b,c) |
|
Alan, in order to proceed I need more information on the following topics:
|
|
While awaiting for Alan's response, I made deep dive into WM code to answer my questions. Here is what I found:
will return all workflows names as a keys, such that we can get full document via:
which brings me different erlang maps, among them the
This output is quite small and will not lead to memory blowup. We can use this view to get list of all workflows in a specific state and then proceed with their documents. @amaltaro , can you agree on such approach? What I propose is the following:
I anticipate that memory footprint will be small in this proposal. To confirm that currently we have in production instance: So, at most we need to load 10K strings in a code (not dictionaries) and them loop over them to load individual workflows document. |
|
@vkuznet it looks like my answer is no longer needed. And I am glad you figured these things out. I do want to make a couple of remarks though:
where |
|
Here is how much memory will be used using the approach I provided: and the output is this: So, for all 10K request in announced state we'll use approximately 1KB of RAM |
This is how I found all details :) Said that, currently we do not expose |
@amaltaro , if you insists of using reqmgr/WMStats please specify which REST end-point we can use. What we have is Web end-point, e.g. |
|
I think you can use followed by |
|
Alan, I know these methods but there are few objections of not using them in this ticket context:
My point is that here we are trying to reduce memory footprint but the memory usage is based on implementation of Therefore, before changing code to use or not |
|
Jenkins results:
|
The motivation is that we cannot track which user is making requests to CouchDB, we do not have the usual kube eagle monitoring as well. Besides, as I already mentioned multiple times in my replies above, we have ReqMgr2 client and REST APIs that we can use for that. There is no need to reinvent anything. |
|
I committed the code which relies on reqmgr2 APIs, but I also pointed out that these APIs involves caching and asked several questions how this cache is handled. I also pointed out that within context of memory allocation we may delegated it to reqmgr2 APIs where cache objects resides and I don't know how it will play out in a global scope. That said, I suggest that you preliminary review my changes and decide if this is correct approach, it uses chunks of requests ids and process them sequentially. For instance for 10K requests, with 100 chunks, and assuming 1sec time to fetch the docs we will need to spend 100sec per iteration. This is due to sequential processing. The larger chunks will lead to larger memory footprints, but smaller chunks to larger overall time. The only way out of it is to introduce the parallel processing. But this is a second round of improvements if we agree on proposed approach with reqmgr2 APIs. |
todor-ivanov
left a comment
There was a problem hiding this comment.
Thanks @vkuznet , The code looks good to me
|
Jenkins results:
|
amaltaro
left a comment
There was a problem hiding this comment.
Just getting this review out of the way.
As discussed a couple of weeks ago, we decided to put this development on hold/waiting and we will probably not resume it this quarter.
Fixes #12200
Status
not-tested
Description
Provide code refactoring to reduce memory footprint
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
<If it's a follow up work; or porting a fix from a different branch, please mention them here.>
External dependencies / deployment changes
<Does it require deployment changes? Does it rely on third-party libraries?>